adding the JsonShapeSerializer#3842
Conversation
| #include <memory> | ||
| #include <aws/core/utils/memory/AWSMemory.h> | ||
|
|
||
| #include "aws/core/client/AWSError.h" |
There was a problem hiding this comment.
nit use: #include <aws/core/client/AWSError.h>
| private: | ||
| Aws::String m_buf; | ||
| int m_depth = 0; | ||
| Aws::Array<bool, MAX_DEPTH> m_needsComma{}; |
|
|
||
| add_library(${PROJECT_NAME} ${AWS_NATIVE_SDK_SRC}) | ||
| add_library(${PROJECT_NAME} ${AWS_NATIVE_SDK_SRC} | ||
| include/smithy/client/schema/ShapeSerializer.h |
There was a problem hiding this comment.
this isnt the right thing to do, look at how we do this for other headers in the smithy directory
| public: | ||
| static void WriteQuotedString(Aws::String& buf, const Aws::String& value); | ||
| private: | ||
| static const bool NEEDS_ESCAPE[128]; |
There was a problem hiding this comment.
if these are static const, why do they need to be static in the WriteQuotedString class? seems like it would be a better fit for them to be a in anonymous namespace in JsonWriteUtils.cpp. also, again, prefer std::array to c style arrays
There was a problem hiding this comment.
moved to anonymous namespace in .cpp, using Aws::Array and switch statement
| ShapeType GetType() const { return m_type; } | ||
| const char* GetId() const { return m_id; } | ||
| const char* GetMemberName() const { return m_memberName; } | ||
| const Aws::String& GetMemberName() const { return m_memberName; } |
There was a problem hiding this comment.
kind of sketched by Aws::String&, it could possibly create lifetime issues however, the way you use it wont. i would err on caution and return Aws::String because likely member name will be short string optimized and removes the lifetime issue not a NEEDS to happen but something to think about.
There was a problem hiding this comment.
changed it to Aws::String
|
|
||
| const char JsonWriteUtils::HEX[16] = { '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', 'a', 'b', 'c', 'd', 'e', 'f'}; | ||
|
|
||
| const bool JsonWriteUtils::NEEDS_ESCAPE[128] = { |
There was a problem hiding this comment.
so we create a table for both needs escape and then the escape value, im not opposed to this however might make more sense just to copy the switch statement from cjson. correct me if im wrong but you more or less could just do that loop in our code, and avoid the size penalty (albeit small) on the translation unit.
2 participants
Adding the implementation for JsonShapeSerializer
Check all that applies:
Check which platforms you have built SDK on to verify the correctness of this PR.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.